Remove NotImplementedError to allow for uploading Zarr assets to embargoed Dandisets
#1540
Conversation
…bargoed Dandisets
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1540 +/- ##
==========================================
+ Coverage 88.44% 88.55% +0.10%
==========================================
Files 78 78
Lines 10691 10718 +27
==========================================
+ Hits 9456 9491 +35
+ Misses 1235 1227 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Hi @jwodder @yarikoptic @jjnesbitt @waxlamp, we should ideally create a new release of |
…rgoed and not dataset I had to duplicate initiation from zarr_dandiset fixture, but seems overall a small price to pay. Subsequent commit would refactor to make sweeping embargoed status on new_dataset easier.
|
We need a test! Pushing with a generic setup to sweep over embargoed or not case easily, and marking few tests with that. |
yarikoptic
force-pushed
the
embargoed-zarr
branch
from
944f91f to
60ccca2
Compare
yarikoptic
added
patch
Co-authored-by: John T. Wodder II <[email protected]>
yarikoptic
force-pushed
the
embargoed-zarr
branch
from
e071cad to
2a1c5ed
Compare
| @@ -250,7 +251,11 @@ def test_upload_bids_non_nwb_file(bids_dandiset: SampleDandiset) -> None: | |||
| assert [asset.path for asset in bids_dandiset.dandiset.get_assets()] == ["README"] | |||
|
|
|||
|
|
|||
| def test_upload_sync_zarr(mocker, zarr_dandiset): | |||
| @sweep_embargo | |||
| def test_upload_sync_zarr(mocker: MockerFixture, zarr_dandiset: SampleDandiset, embargo: bool) -> None: | |||
There was a problem hiding this comment.
The linter is complaining that this line is too long. You don't seem to have pre-commit set up in your local clone.
There was a problem hiding this comment.
I do have precommit -- I just accepted your suggestion online and that caused it -- we do not have pre-commit on CI to fix things up. I have pushed fixed up rewrite I did locally. Should be all good now.
|
@kabilar have a final look on how now we can decorate tests to test against embargoed and not for future references. I will merge (it will release, labels added) after all green. |
Thank you, @yarikoptic. That sounds good. It looks like one workflow is failing as described in #1541. |
|
Let's proceed. I will add more info on that issue |
|
🚀 PR was released in |
cc @jjnesbitt @aaronkanzer @waxlamp